Conversation
This allows a user to retry an attempted copy if it requires the overwrite option.
WalkthroughThe changes update the file copying functionality within the directory preview component. A new type, Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/app/view/preview/directorypreview.tsx (3)
39-43: Consider extending or enumerating error handling states.
The new "FileCopyStatus" type is adequate but might benefit from distinguishing various error types or including error codes, allowing more robust handling of multiple failure scenarios.
912-931: Enhance error checking logic.
Relying on “.endsWith('overwrite not specified')” could be fragile if API error messages change. Consider a more structured approach (e.g., error codes) for robust retry checks.
963-964: Confirm complete dependency array.
If "handleDropCopy" is itself a dependency, consider adding it explicitly to align with React’s best practices, ensuring the drop callback always has the latest version of the function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/view/preview/directorypreview.scss(1 hunks)frontend/app/view/preview/directorypreview.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
frontend/app/view/preview/directorypreview.tsx (5)
5-5: Import usage looks good.
This added import for "CopyButton" is used within the new overlay. No further issues found.
7-7: Confirmed reference usage of custom hook.
The imported hook "useDimensionsWithCallbackRef" is applied properly in the overlay. No concerns.
801-801: Ensure the type accommodates null state.
While you’ve assigned null, verify that the "FileCopyStatus" type or usage pattern expects or safely handles null. You could consider using "FileCopyStatus | null" to make this safe explicitly.
958-958: Drop callback chaining is well-handled.
"handleDropCopy" is cleanly invoked upon a drop event, providing a clear flow. No issues.
1077-1083: Conditional overlay rendering is straightforward.
Displaying the "CopyErrorOverlay" when "copyStatus" is non-null is logical and succinct.frontend/app/view/preview/directorypreview.scss (1)
203-319: Overlay styling is cohesive and responsive.
These new ".copyerror-overlay" rules appear well-structured and consistent. Keep an eye on potential performance implications of a high blur radius. Otherwise, looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/app/view/preview/directorypreview.tsx (3)
39-43: Add JSDoc comments to document the type definition.The
FileCopyStatustype would benefit from documentation explaining the purpose of each field.+/** + * Represents the status of a file copy operation. + */ type FileCopyStatus = { + /** The data required for the copy operation */ copyData: CommandFileCopyData; + /** Error message if the copy operation failed */ copyError: string; + /** Whether the operation can be retried with overwrite flag */ allowRetry: boolean; };
912-931: Improve error handling in the copy operation.The error handling could be more robust:
- Consider using a more specific error type instead of converting to string directly.
- Add error classification to handle different types of errors differently.
const handleDropCopy = useCallback( async (data: CommandFileCopyData) => { try { await RpcApi.FileCopyCommand(TabRpcClient, data, { timeout: data.opts.timeout }); setCopyStatus(null); } catch (e) { console.log("copy failed:", e); - const copyError = `${e}`; - const allowRetry = copyError.endsWith("overwrite not specified"); + // Classify error types + const copyError = e instanceof Error ? e.message : String(e); + const allowRetry = copyError.includes("overwrite not specified") || + copyError.includes("file exists"); const copyStatus: FileCopyStatus = { copyError, copyData: data, allowRetry, }; setCopyStatus(copyStatus); } model.refreshCallback(); }, [setCopyStatus, model.refreshCallback] );
1135-1140: Improve error message presentation.The error message handling could be more user-friendly and consistent.
- let statusText = "Copy Error"; - let errorMsg = `error: ${copyStatus?.copyError}`; + let statusText = copyStatus?.allowRetry ? "Confirm Overwrite" : "Copy Failed"; + let errorMsg = copyStatus?.allowRetry + ? "One or more files already exist at the destination. Would you like to overwrite them?" + : `Failed to copy: ${copyStatus?.copyError}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/preview/directorypreview.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
frontend/app/view/preview/directorypreview.tsx (1)
1124-1133: Avoid direct mutation of copyData in handleRetryCopy.The function should create a new object instead of mutating the existing one.
| const handleCopyToClipboard = React.useCallback( | ||
| async (e: React.MouseEvent) => { | ||
| await navigator.clipboard.writeText(errorMsg); | ||
| }, | ||
| [errorMsg] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to clipboard operations.
The clipboard operation should handle potential failures gracefully.
const handleCopyToClipboard = React.useCallback(
- async (e: React.MouseEvent) => {
+ async () => {
try {
await navigator.clipboard.writeText(errorMsg);
+ } catch (error) {
+ console.error('Failed to copy to clipboard:', error);
+ // Optionally show a user-friendly error message
}
},
[errorMsg]
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleCopyToClipboard = React.useCallback( | |
| async (e: React.MouseEvent) => { | |
| await navigator.clipboard.writeText(errorMsg); | |
| }, | |
| [errorMsg] | |
| ); | |
| const handleCopyToClipboard = React.useCallback( | |
| async () => { | |
| try { | |
| await navigator.clipboard.writeText(errorMsg); | |
| } catch (error) { | |
| console.error('Failed to copy to clipboard:', error); | |
| // Optionally show a user-friendly error message | |
| } | |
| }, | |
| [errorMsg] | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/app/view/preview/directorypreview.tsx (3)
912-931: Enhance error handling with specific error types and descriptive messages.While the error handling is functional, it could be improved for better user experience and debugging.
Consider these improvements:
async (data: CommandFileCopyData) => { try { await RpcApi.FileCopyCommand(TabRpcClient, data, { timeout: data.opts.timeout }); setCopyStatus(null); } catch (e) { - console.log("copy failed:", e); - const copyError = `${e}`; + console.error("File copy operation failed:", e); + const errorMessage = e instanceof Error ? e.message : String(e); + const isOverwriteError = errorMessage.endsWith("overwrite not specified"); + const copyError = isOverwriteError + ? "Cannot overwrite existing file without permission" + : `File copy failed: ${errorMessage}`; const allowRetry = copyError.endsWith("overwrite not specified"); const copyStatus: FileCopyStatus = { copyError, copyData: data, allowRetry, }; setCopyStatus(copyStatus); } model.refreshCallback(); }
1124-1133: Optimize retry function memoization.The retry function's dependency array could be optimized to prevent unnecessary re-renders.
const handleRetryCopy = React.useCallback(async () => { if (!copyStatus) { return; } const updatedData = { ...copyStatus.copyData, opts: { ...copyStatus.copyData.opts, overwrite: true }, }; await handleDropCopy(updatedData); -}, [copyStatus.copyData]); +}, [copyStatus?.copyData, handleDropCopy]);
1153-1204: Enhance accessibility with ARIA labels and keyboard navigation.The error overlay could be more accessible with proper ARIA attributes and keyboard support.
<div ref={overlayRefCallback} + role="dialog" + aria-label={statusText} + aria-describedby="error-message" className="absolute top-[0] left-1.5 right-1.5 z-[var(--zindex-block-mask-inner)] overflow-hidden bg-[var(--conn-status-overlay-bg-color)] backdrop-blur-[50px] rounded-md shadow-lg" > {/* ... */} - <div>{errorMsg}</div> + <div id="error-message">{errorMsg}</div> {/* ... */} <Button className={buttonClassName} + onKeyDown={(e) => { + if (e.key === 'Escape') { + handleRemoveCopyError(); + } + }} onClick={handleRemoveCopyError} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/preview/directorypreview.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
frontend/app/view/preview/directorypreview.tsx (2)
39-43: LGTM! Well-structured type definition and state management.The
FileCopyStatustype effectively captures all necessary information for tracking file copy operations, and the state management is properly implemented using React's useState hook.Also applies to: 801-801
1148-1150: Add error handling to clipboard operations.The clipboard operation should handle potential failures gracefully.
const handleCopyToClipboard = React.useCallback(async () => { + try { await navigator.clipboard.writeText(errorMsg); + } catch (error) { + console.error('Failed to copy to clipboard:', error); + // Optionally show a user-friendly error message + } }, [errorMsg]);
If a file drag and drop file fails because the file already exists, this adds a popup to allow the operation to be retried with the overwrite flag set. Additionally, it will make a similar dismissible popup to cover other copy errors.
If a file drag and drop file fails because the file already exists, this adds a popup to allow the operation to be retried with the overwrite flag set. Additionally, it will make a similar dismissible popup to cover other copy errors.